feat: add firebase functions v2 preset #1142
Conversation
β Live Preview ready!
|
Codecov Report
@@ Coverage Diff @@
## main #1142 +/- ##
==========================================
- Coverage 76.82% 76.60% -0.22%
==========================================
Files 71 72 +1
Lines 7317 7301 -16
Branches 729 727 -2
==========================================
- Hits 5621 5593 -28
- Misses 1695 1707 +12
Partials 1 1
|
|
@danielroe any chance of looking at this one? Would really appreciate your advice on using runtimeConfig to pass options through - I've not seen this done yet in nitro and wonder if it's the correct approach? runtimeConfig: {
firebaseV2: {
httpRequestOptions: <HttpsOptions>{
region: "europe-west1",
maxInstances: 3
}
}
}It could go up one level into the nitro config |
|
|
|
@sisou I've updated this PR to add the firebase functions options to the nitro config. I've added the firebase-functions package to import the types from because it seems a bit mental to be copying them over... Let me know if this is the correct approach. CC @danielroe |
|
I've just merged main in. I had a merge conflict in the Is there anything else I need to do before this is ready to go in @danielroe @pi0 @sisou ? |
|
I'm not a maintainer of this project, just an interested user π |
|
@danielroe @pi0 , anything I need to do to get this moving? Would be great to start using it πππ€ |
|
@Hebilicious - I've added a line in the docs about the functions permissions pointing the user to the google cloud console π |
|
@pi0 Thanks for the review π Should all be amended now π |
There was a problem hiding this comment.
Hello,
Disclaimer: I'm the maintainer of VueFire and occasionally work with Firebase.
One of the things we were planning to release with Firebase is a preset for nitro that handles the different generations. I think this is a great addition as it will unblock anybody trying to use the faster version of Cloud functions π
However, I don't think it's a good idea to name this firebase-v2 because it sounds as the successor of the firebase preset but if there is a 3rd generation of cloud functions, there will be a firebase-v3 preset and so on. In practice, 1st and 2nd generation cloud functions can be used on the same project so I think it's better to name it something else like firebase-2nd-gen and release it as a temporary and experimental preset. Even better, release it as a separate package so you have full control over it and unblock yourself and other users.
Because of that, I also think it's better not to document it just like another preset as the goal should be to make the firebase one work in all scenarios.
In the future, I think having the official nitro firebase preset as a separate package will be better as it will allow the preset to have breaking changes such as defaulting to the 2nd generation of cloud functions.
|
|
||
| export default defineNitroConfig({ | ||
| // Nitro options... | ||
| firebase: { |
There was a problem hiding this comment.
I think it's better to name this something different to keep the firebase config option for the firebase preset only. It will avoid any possible conflicts.
There was a problem hiding this comment.
I'll let @pi0 weigh in on this, as we've already been through a review on it. See the resolved issue here @posva:
#1142 (review)
There was a problem hiding this comment.
Just my 2 cents, but for #1377 my idea was to have the config option shared between v1 and v2 functions, so you could seamlessly switch between them without worrying about the config. As someone using this in production, it would be nice to switch between v1 and v2 without worrying about accidentally overwriting function deployments.
There was a problem hiding this comment.
@AlexLavoie42 Why would you switch between 1st and 2nd gen in production? Normally one would upgrade to gen 2 because they are better but they would never switch back. Even more when talking about the function that handles SSR. Or are you talking about upgrading too? In that case, I also think adding the same options where possible would make sense
There was a problem hiding this comment.
You're right, the vast majority of the time it will be just upgrading from v1 to v2, although I could see some circumstances where we would downgrade to v1 temporarily for debugging/testing purposes.
The main purpose behind sharing the config would be to simply upgrade to v2 without changing config. I have not actually tested this, but I assume publishing a v1 and v2 function with the same name will overwrite each other? If the config is separated there is definitely a possibility that when upgrading users will forget to move the config over and overwrite the default server function.
There was a problem hiding this comment.
Could you detail those circumstances? I don't think I see any reason not to upgrade the currently named server function that renders the Nuxt app to gen 2 and then rollback to gen 1 π€
Publishing a gen 2 function with the same name will error, you have to name it differently
The main purpose behind sharing the config would be to simply upgrade to v2 without changing config.
Totally π
There was a problem hiding this comment.
Publishing a gen 2 function with the same name will error, you have to name it differently
This is good to know, so in that case you would not be able upgrade to v2 with the same function name unless you delete the original first? Perhaps in that case it should actually be a different config value to avoid an error. Maybe even the default name should be different as well then?
As for downgrading to v1, I am imagining a case where we downgrade temporarily either to debug or rollback in case of any bugs that arise after upgrading. Granted, I don't think this scenario is very likely.
There was a problem hiding this comment.
IMO the name should be different yes.
so in that case you would not be able upgrade to v2 with the same function name unless you delete the original first?
Not exactly: upgrading is a bit different as you want to go through a one-time step where you have both gen functions co-existing. In more complex scenarios: https://firebase.google.com/docs/functions/2nd-gen-upgrade.
You could also just redeploy the second gen with a different name and remove the 1st one in one deployment. I'm not completely sure but I imagine you risk losing a fraction of traffic during that deployment, which in some projects is fine, in others isn't.
| @@ -65,4 +67,13 @@ export interface PresetOptions { | |||
| maxDuration: number; | |||
| }; | |||
| }; | |||
| firebase: { | |||
There was a problem hiding this comment.
What would be the difference with runtimeConfig? I see other presets using useRuntimeConfig() but none using useAppConfig()
There was a problem hiding this comment.
@posva I guess you're referencing this line where we add the config to the appConfig. The only reason we do this is to be able to reference it in the entry file here:
https://github.com/unjs/nitro/pull/1142/files#diff-89af85ba779addbc7c61eaecf298b36da9e6d1d5753f25ff55416ef9f50d833eR8
It's all put under a key that isn't intended to be used by the end user. This was implemented on @Hebilicious suggestion here:
#1142 (comment)
We took the options out of runtimeConfig to put them along side other preset config like vercel:
#1142 (comment)
Nitro options seem like the best place to put these as you are configuring the nitro build at this point rather than runtime options. Then I guess putting them either in app config or runtime config doesn't really matter as it's just a way to get those options into the entry file?
Happy to change it though if it's better practice @Hebilicious @pi0 ?
There was a problem hiding this comment.
Thanks! This really helps to follow the flow of the PR!
There was a problem hiding this comment.
@Hebilicious any thoughts on this?
There was a problem hiding this comment.
@Hebilicious any thoughts on this?
Everything looked good to me the last time I reviewed it, I believe @posva is adding automated tests in nitro deploys and @pi will be in charge of merging.
|
Thanks all for your contribution to the firebase v2 preset <3 Let's track from one PR #1500 and hopefully soon main branch. |
π Linked issue
#1141
β Type of change
π Description
This adds a new preset for firebase functions v2 - the second generation of firebase functions that is now in GA.
The preset name is
firebaseV2though I'm not sure if it should befirebase-v2orfirebase-nextorfirebase-gen-2π€· .I've just extended the existing firebase preset as much of the functionality / requirements are the same, just the entry file needs to import functions from a slightly different place and we now have the option of passing in an object to configure the function which is super helpful, rather than chaining.
The new version of functions has many new features and will, no doubt, be the default version soon now that it's in GA. One of the main features is the ability to take concurrent requests which is a big win for apps running on cloud functions so that there is less cold start time.
Using v2 could functions should see a speed improvement and gives the developer more flexibility and control over the server.
Resolves #1141
Resolves #666
Resolves #90
π Checklist
I'm not 100% on the convention on passing config to a preset. Other presets advocate a config file, that is read during deploy, but with firebase functions it seems that the configuration needs to be in the code. I've used the runtimeConfig object to pass down config. Seems like a sensible place, but I understand that it may also need documentation / changes to the types and I'm not sure that's the road you want to go down...
There were no tests for the firebase preset, so I've not made any tests for this firebaseV2 preset. I have tested that it works in a nuxt3 app and it works great. Lemme know on tests π